Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix IPv6 Port Forwarding for the Bridge Driver #2604

Merged
merged 1 commit into from
Dec 15, 2020

Conversation

arkodg
Copy link
Contributor

@arkodg arkodg commented Dec 14, 2020

  1. Allocate either a IPv4 and/or IPv6 Port Binding (HostIP, HostPort, ContainerIP,
    ContainerPort) based on the input and system parameters
  2. Update the userland proxy as well as dummy proxy (inside port mapper) to
    specifically listen to either the IPv4 or IPv6 network

Signed-off-by: Arko Dasgupta [email protected]

@arkodg arkodg requested a review from thaJeztah December 14, 2020 00:04
@arkodg
Copy link
Contributor Author

arkodg commented Dec 14, 2020

cc @bboehmke

Copy link
Contributor

@bboehmke bboehmke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks far better and cleaner as my try.

pb, err := n.allocatePortsInternal(ep.extConnConfig.PortBindings, ep.addr.IP, defHostIP, ulPxyEnabled)
if err != nil {
return nil, err
var containerIPv6 net.IP = nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to set containerIPv6 explicit to nil?

var containerIPv6 net.IP should already be nil by default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack, thanks

}

// IPv6 port binding excluding user land proxy
if n.driver.config.EnableIP6Tables && ep.addrv6 != nil {
Copy link
Contributor

@bboehmke bboehmke Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The EnableIP6Tables check was removed completely.

Is there anything that prevents a creation of ip6tables rules in AppendForwardingTableEntry of the portmapper if EnableIP6Tables is not set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If

n.portMapperV6.SetIptablesChain(natChain, n.getNetworkBridgeName())

is skipped, pm.chain will be nil and will skip the iptables programming
if pm.chain == nil {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I see. That is for sure far cleaner than before.

@arkodg arkodg changed the title Fix Port Forwarding for the Bridge Driver Fix IPv6 Port Forwarding for the Bridge Driver Dec 15, 2020
@arkodg arkodg force-pushed the fix-port-forwarding branch 2 times, most recently from 75f4191 to f0cba0e Compare December 15, 2020 02:30
1. Allocate either a IPv4 and/or IPv6 Port Binding (HostIP, HostPort, ContainerIP,
ContainerPort) based on the input and system parameters
2. Update the userland proxy as well as dummy proxy (inside port mapper) to
specifically listen on either the IPv4 or IPv6 network

Signed-off-by: Arko Dasgupta <[email protected]>
@arkodg arkodg force-pushed the fix-port-forwarding branch from f0cba0e to 28576a4 Compare December 15, 2020 02:46
@bboehmke
Copy link
Contributor

LGTM

@arkodg
Copy link
Contributor Author

arkodg commented Dec 15, 2020

thanks @bboehmke can you please formally approve the PR, so I can merge it and vendor it into moby/moby

@arkodg arkodg merged commit fa125a3 into moby:master Dec 15, 2020
@arkodg arkodg deleted the fix-port-forwarding branch December 15, 2020 16:25
@bboehmke bboehmke mentioned this pull request Dec 15, 2020
3 tasks
arkodg pushed a commit to arkodg/moby that referenced this pull request Dec 15, 2020
Brings in moby/libnetwork#2604

Signed-off-by: Arko Dasgupta <[email protected]>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Dec 23, 2020
Brings in moby/libnetwork#2604

Signed-off-by: Arko Dasgupta <[email protected]>
Upstream-commit: 78eafdd947db80832c6d0985c81cb56944a23739
Component: engine
listener, err := sctp.ListenSCTP("sctp", frontendAddr)
// detect version of hostIP to bind only to correct version
ipVersion := ipv4
if frontendAddr.IPAddrs[0].IP.To4() == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we currently don't support IPv4-mapped IPv6 addresses today, we build out a separate ipv4 and ipv6 stack

@vin01
Copy link

vin01 commented Jan 5, 2021

1. Allocate either a IPv4 and/or IPv6 Port Binding (HostIP, HostPort, ContainerIP,
   ContainerPort) based on the input and system parameters

2. Update the userland proxy as well as dummy proxy (inside port mapper) to
   specifically listen to either the IPv4 or IPv6 network

Signed-off-by: Arko Dasgupta [email protected]

This looks like a breaking change since until now docker-proxy has been binding to both v4 and v6 addresses if both are available on the system which I believe made sense.

@fbezdeka
Copy link

fbezdeka commented Jan 5, 2021

According to the Changelog [1] this is the only network related change in 20.10.2.
Now my IPv6 port forwarding is completely broken.
Downgrading to 20.10.1 fixed it.

There are no docker-proxy processes started. 20.10.1 starts a process for each port-forwarding.

Example:

sudo docker run \
    --hostname gitlab \
    --env 'GITLAB_PORT=443' \
    --env 'GITLAB_HTTPS=true' \
    --publish [<ip>]:2222:22 \
    --publish [<ip>]:8080:80 \
    --publish [<ip>]:8443:443 \
    --name gitlab \
    --restart always \
    --volume /srv/gitlab/config:/etc/gitlab:Z \
    --volume /srv/gitlab/logs:/var/log/gitlab:Z \
    --volume /srv/gitlab/data:/var/opt/gitlab:Z \
    gitlab/gitlab-ce:latest

[1] https://docs.docker.com/engine/release-notes/

@pokotiX
Copy link

pokotiX commented Jan 5, 2021

According to the Changelog [1] this is the only network related change in 20.10.2.
Now my IPv6 port forwarding is completely broken.
Downgrading to 20.10.1 fixed it.

There are no docker-proxy processes started. 20.10.1 starts a process for each port-forwarding.

Example:

sudo docker run \
    --hostname gitlab \
    --env 'GITLAB_PORT=443' \
    --env 'GITLAB_HTTPS=true' \
    --publish [<ip>]:2222:22 \
    --publish [<ip>]:8080:80 \
    --publish [<ip>]:8443:443 \
    --name gitlab \
    --restart always \
    --volume /srv/gitlab/config:/etc/gitlab:Z \
    --volume /srv/gitlab/logs:/var/log/gitlab:Z \
    --volume /srv/gitlab/data:/var/opt/gitlab:Z \
    gitlab/gitlab-ce:latest

[1] https://docs.docker.com/engine/release-notes/

I confirm, I have the same problem

@arkodg
Copy link
Contributor Author

arkodg commented Jan 5, 2021

@pokotiV @fbezdeka @vin01 can you please raise a separate issue to discuss this
thanks

@terencehonles
Copy link

@pokotiV @fbezdeka @vin01 can you please raise a separate issue to discuss this
thanks

@arkodg done in #2607 (I believe the title captures the problem that everyone was seeing (it does mine))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants